Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle warnings while creating service broker #1359

Conversation

AartiKriplani
Copy link
Contributor

This PR, introduces warnings header X-Cf-Warnings on a v3 endpoint/controller for the first time.

More info #166186412

@cfdreddbot
Copy link

✅ Hey AartiKriplani! The commit authors and yourself have already signed the CLA.

@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/166409056

The labels on this github issue will be updated when the story is started.

Copy link
Contributor

@piyalibanerjee piyalibanerjee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! The implementation looks great -- especially that it's mainly added to the application_controller so that any controller inheriting from it would be able to use it. Here are some comments/questions I had while reviewing this:

  1. VAT's been placing all the V3 actions directly in the app/actions directory rather than in app/actions/v3. We can discuss further whether we should do some moving around to either get rid of the v3 folder completely or to move everything into the existing v3 folder.
  2. I saw that the warnings array check was being done at the application controller level -- I haven't seen ArgumentErrors being raised in any other controllers so I was wondering whether the array check can be done somewhere else? Like when warnings are being created?

[finishes #166186412]
[PR]

Co-authored-by: Oleksii Fedorov <[email protected]>
Co-authored-by: George Blue <[email protected]>
@waterlink
Copy link
Contributor

waterlink commented Jun 4, 2019

@psparks123

VAT's been placing all the V3 actions directly in the app/actions directory rather than in app/actions/v3. We can discuss further whether we should do some moving around to either get rid of the v3 folder completely or to move everything into the existing v3 folder.

Makes sense, we’ll fix that.

I saw that the warnings array check was being done at the application controller level -- I haven't seen ArgumentErrors being raised in any other controllers so I was wondering whether the array check can be done somewhere else? Like when warnings are being created?

This is an error not for the user/client, but for a developer to not accidentally pass different types that have :each defined on them.

Like when warnings are being created?

Not sure how to do that. The point is that action code will create these arrays and return them to the controller, and controller will pass that to the add_warning_headers with that.

And actions should be creating arrays properly there. The check is only a sanity check that will inform the developer what they are doing wrong.

Another option is to create a separate Warnings class that all actions have to create. And that can do the validation at construction time. That might be too complex for the current set of problems though…

@AartiKriplani AartiKriplani force-pushed the v3-create-service-broker-warnings-166186412-1 branch from ee9f019 to c866362 Compare June 4, 2019 10:16
@AartiKriplani AartiKriplani merged commit 7a6413d into cloudfoundry:master Jun 6, 2019
@AartiKriplani AartiKriplani deleted the v3-create-service-broker-warnings-166186412-1 branch June 6, 2019 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants